Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Sync analysis_options.yaml with flutter/flutter#34986

Merged
goderbauer merged 14 commits into
flutter-team-archive:mainfrom
goderbauer:syncAnalysisOptions
Jul 29, 2022
Merged

Sync analysis_options.yaml with flutter/flutter#34986
goderbauer merged 14 commits into
flutter-team-archive:mainfrom
goderbauer:syncAnalysisOptions

Conversation

@goderbauer

Copy link
Copy Markdown
Contributor

No description provided.

@goderbauer goderbauer force-pushed the syncAnalysisOptions branch from 229c1f8 to 416ca7b Compare July 29, 2022 00:23

@chinmaygarde chinmaygarde left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! So much better.

@goderbauer

Copy link
Copy Markdown
Contributor Author

@yjbanov Can you scan through the changes this required in web_ui?

@goderbauer goderbauer requested a review from yjbanov July 29, 2022 21:30
@goderbauer goderbauer force-pushed the syncAnalysisOptions branch from ac7e503 to 9bbdb8c Compare July 29, 2022 22:07
@goderbauer

Copy link
Copy Markdown
Contributor Author

@dnfield Would you be up for reviewing the changes to lib/ui that this required now that we are properly analyzing it after #34988 got resolved? Those changes are all in the last commit of this PR. All other commits have already been reviewed.

@goderbauer goderbauer requested a review from dnfield July 29, 2022 22:09
Comment thread analysis_options.yaml
missing_return: warning
native_function_body_in_non_sdk_code: ignore
# allow having TODOs in the code
native_function_body_in_non_sdk_code: ignore # DIFFERENT FROM FLUTTER/FLUTTER

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need this anymore now that we're on FfiNative.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind if I remove this in a follow-up?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to turn this on in #35015 to see what (if anything) breaks.

Comment thread analysis_options.yaml Outdated
# Turned off until null-safe rollout is complete.
unnecessary_null_comparison: ignore
# TODO(goderbauer): remove when https://github.com/dart-lang/sdk/issues/49563 is fixed.
ffi_native_unexpected_number_of_parameters: ignore

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is different from flutter right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, will leave a comment on it.

Comment thread analysis_options.yaml
- avoid_return_types_on_setters
# - avoid_returning_null # there are plenty of valid reasons to return null
# - avoid_returning_null_for_future # not yet tested
# - avoid_returning_null # still violated by some pre-nnbd code that we haven't yet migrated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What code have we not migrated? Can we make sure an issue is filed for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just syncing over from flutter/flutter and that's what we have there. Happy to take another look at this particular lint in a follow-up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, we should be fully NNBD in this repo (except for flutter_frontend_server and a couple stray files in fuchsia embedding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to turn this on in #35015 to see what (if anything) breaks.

@dnfield dnfield left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is overall an improvement, feel free to land and file issues/follow up on my comments subsequently to avoid merge conflicts with bots

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 29, 2022
@dnfield

dnfield commented Jul 29, 2022

Copy link
Copy Markdown
Contributor

@dnfield Would you be up for reviewing the changes to lib/ui that this required now that we are properly analyzing it after #34988 got resolved? Those changes are all in the last commit of this PR. All other commits have already been reviewed.

LGTM

@goderbauer goderbauer merged commit 05ff95d into flutter-team-archive:main Jul 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Development

Successfully merging this pull request may close these issues.

5 participants